Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Field Status metadata to Online Serving #658

Merged
merged 65 commits into from
Jun 19, 2020
Merged

Add Field Status metadata to Online Serving #658

merged 65 commits into from
Jun 19, 2020

Conversation

mrzzy
Copy link
Collaborator

@mrzzy mrzzy commented Apr 27, 2020

What this PR does / why we need it:
Adds field status metadata to Online Serving so that users can debug Serving returning missing values.

Field status metadata returned (FieldStatus) classifies the missing values into 3 possible cases:

  • NULL_VALUE - the value is missing because the user did not specify a value on ingestion.
  • NOT_FOUND - the value is missing because it could not be retrieved from the online store (ie feature data key not found Redis store.)
  • OUTSIDE_MAX_AGE - the value is missing because it was purposely discarded as the time since the value was ingested and retrieval has exceeded the FeatureSet's max age.

Updates SDKs (Go, Java, Python) to support field status metadata.

Which issue(s) this PR fixes:

Fixes #278

Does this PR introduce a user-facing change?:

Add Field Status metadata to Online Serving:
* changes to ServingService Protobuf:
  - Added statuses map to FieldValues.
  - Added FieldStatus enum to define Field Status states.
* Updated SDKs to support Field Status Metadata.

@mrzzy
Copy link
Collaborator Author

mrzzy commented Apr 28, 2020

/test test-serving

@mrzzy
Copy link
Collaborator Author

mrzzy commented Apr 28, 2020

/test test-python-sdk

@mrzzy
Copy link
Collaborator Author

mrzzy commented Apr 28, 2020

/test test-golang-sdk

@mrzzy
Copy link
Collaborator Author

mrzzy commented Apr 28, 2020

/test test-end-to-end

@mrzzy
Copy link
Collaborator Author

mrzzy commented Apr 28, 2020

/test test-end-to-end-redis-cluster

@mrzzy mrzzy changed the title WIP: Add feature metadata to Online Serving Add feature metadata to Online Serving Apr 28, 2020
@mrzzy mrzzy marked this pull request as ready for review April 28, 2020 10:22
@woop woop mentioned this pull request Apr 29, 2020
@ches
Copy link
Member

ches commented Apr 29, 2020

Just a very, um, meta thing—if I saw this changelog and #536 in the same release notes I would think they're related. Should we find some disambiguating terminology? Maybe "registry metadata" and "serving metadata"?

@ches
Copy link
Member

ches commented Apr 29, 2020

I guess "feature (set) labels" could already suffice unambiguously for what I called "registry metadata". The feature in this PR being called "feature metadata" is problematic though, I think.

@woop woop added this to the v0.6.0 milestone Apr 29, 2020
@woop
Copy link
Member

woop commented Apr 29, 2020

Just a very, um, meta thing—if I saw this changelog and #536 in the same release notes I would think they're related. Should we find some disambiguating terminology? Maybe "registry metadata" and "serving metadata"?

Yea I agree :)

Actually the Feast project has a long history of abusing the term metadata. It's just such a convenient word. It's everything Util, Spec, and Info aspire to be, and more.

How about

  • Feature status metrics
  • Feature status metadata
  • Feature value statuses <- my preference.

@ches ches added the compat/breaking Breaking user-facing change label Apr 29, 2020
@ches
Copy link
Member

ches commented Apr 29, 2020

It's everything Util, Spec, and Info aspire to be, and more.

I might steal that.

I think "feature value statuses" is apt.

@ches
Copy link
Member

ches commented Apr 29, 2020

Can we introduce this change/functionality in an evolutionary-friendly way? Like introduce it as a new RPC and new messages, with the old ones remaining supported, possibly going through a deprecation period before Feast 1.0?

Breaking the contract is extremely disruptive here. Upgrading the Serving API will break existing online clients in production, they will all have to upgrade their SDK, and it's not possible to roll out a forward-compatible SDK upgrade to fleets in advance. This breaks all the rules of API evolution.

@woop
Copy link
Member

woop commented Apr 30, 2020

@mrzzy any objections to feature value statuses?

Breaking the contract is extremely disruptive here. Upgrading the Serving API will break existing online clients in production, they will all have to upgrade their SDK, and it's not possible to roll out a forward-compatible SDK upgrade to fleets in advance. This breaks all the rules of API evolution.

We can. Some ideas on gRPC versioning from Microsoft.

We could add a new RPC with V2?

@mrzzy mrzzy changed the title Add feature metadata to Online Serving Add Field Status metadata to Online Serving Apr 30, 2020
@mrzzy
Copy link
Collaborator Author

mrzzy commented Apr 30, 2020

I renaming "Feature Metadata" to "Field Status" in the PR name instead of "Feature Status" as it also returns entity statuses in addition to feature statuses.

@zhilingc
Copy link
Collaborator

The golang code has to be versioned too, since version pinning is a non-goal of go modules :/

@ches
Copy link
Member

ches commented Apr 30, 2020

This is a good example I think in favor of SDKs providing higher-level API wrapping protobuf types and request/response objects—this Java API was seamlessly upgraded in the PR, and the same could apply to using a new RPC underneath and phasing out the old one:

public List<Row> getOnlineFeatures(List<String> features, List<Row> rows, String defaultProject)

while this is very susceptible to source-incompatible breakage, at least if user code touches the (public) underlying RawResponse:

func (fc *GrpcClient) GetOnlineFeatures(ctx context.Context, req *OnlineFeaturesRequest) (
	*OnlineFeaturesResponse, error) {

@zhilingc
Copy link
Collaborator

zhilingc commented Apr 30, 2020

The golang SDK follows a similar pattern, but unfortunately for golang the proto-generated code is persisted (and versioned) together with the sdk code :(

type Client interface {
	GetOnlineFeatures(ctx context.Context, req *OnlineFeaturesRequest) (*OnlineFeaturesResponse, error)
}

@woop woop added the kind/feature New feature or request label May 3, 2020
mrzzy added 22 commits June 18, 2020 21:51
…Row> instead of returning null.

This allows the code to more semenatic and readable compared to dealing
with nullable FeatureRow values
@woop
Copy link
Member

woop commented Jun 19, 2020

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrzzy, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop
Copy link
Member

woop commented Jun 19, 2020

/test test-end-to-end-batch

@feast-ci-bot feast-ci-bot merged commit eebf458 into feast-dev:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return missing feature metadata in response
6 participants